Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Reject programs with unconditional recursion #6292

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

aakoshh
Copy link
Contributor

@aakoshh aakoshh commented Oct 15, 2024

Description

Problem*

Related to #4828
Followup to #6291

Summary*

Added a new ResolutionError::UnconditionalRecursion which should result in a warning or compilation error where the function body recurses into the defining function with no alternative way to return.

Additional Context

Documentation*

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [For Experimental Features] Documentation to be submitted in a separate PR.

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@aakoshh aakoshh marked this pull request as draft October 16, 2024 00:19
@aakoshh aakoshh changed the title feat: Unconditional recursion error feat: Reject programs with unconditional recursion Oct 16, 2024
@aakoshh aakoshh force-pushed the 4828-unconditional-recursion-error branch from 6e905db to cf10d59 Compare October 16, 2024 00:54
@aakoshh aakoshh marked this pull request as ready for review October 16, 2024 01:10
@asterite
Copy link
Collaborator

I wonder if this error should be produced on the SSA side instead of panicking when the "too much recursion" is detected. That way it would detect mutual recursion too.

@aakoshh
Copy link
Contributor Author

aakoshh commented Oct 16, 2024

You mean to change the return value into Result? I didn’t investigate the impact, or if it has any other panics that could be covered. There was another PR with a similar case where I changed the panic message and added some CLI side filtering, but didn’t want to cause too much churn by changing the entire call stack.

I’ll defer to @TomAFrench

@aakoshh aakoshh force-pushed the 4828-unconditional-recursion-error branch from 6cd6c11 to 0e29ea2 Compare October 16, 2024 17:46
@aakoshh aakoshh changed the base branch from 4828-inline-recur-limit-panic-msg to master October 16, 2024 17:46
@aakoshh
Copy link
Contributor Author

aakoshh commented Oct 16, 2024

Ah, you mean that instead of a warning, we could rely purely on the SSA.

I did it this way so it can be a warning during compilation, like the unused variables. Rust has this warning too, so why not? I didn't implement mutual recursion because Rust doesn't seem to have it either, but in theory I think we could: if we detect a cycle where none of the code paths can return without calling any of the FuncId we collected along the way, then it could reject that case too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants